-
Notifications
You must be signed in to change notification settings - Fork 0
DM-48010: Add directions to playbook for Science Pipelines quick-builds #242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1bc104b
to
62e1136
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates (especially the list format, which I didn't know how to do)! However, aside from my question of whether the quick-stack instructions belong here at all, I think some of the details have gotten mixed up. I'd like to see those resolved before I approve.
doc/playbook.rst
Outdated
@@ -67,6 +67,36 @@ Any subsequent builds of the service container will build against both bases. | |||
|
|||
This is the only situation in which a change to ``BASE_TAG_LIST`` should be committed to ``main``. | |||
|
|||
In case your intended Base Container is not a tagged Science Pipelines Release: Building the Science Pipelines Manually | |||
----------------------------------------------------------------------------------------------------------------------- | |||
It will sometimes be necessary to compile a container with the LSST Science Pipelines manually. Generally, this only occurs if the intended daily or weekly stack does not compile. In these cases, the Science Pipelines themselves must be built ahead of the base container. Here are instructions for building the Science Pipelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that, since this is not Prompt Processing functionality, it should be documented elsewhere (https://github.com/lsst/gha_build/?) and merely linked from here. That would keep our documentation focused (see DMTN-030), and would make the build documentation visible to other users of the quick-stack system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write (at most) one sentence per line, per the dev guide. That makes for much cleaner diffs in future editing; otherwise, changing one word might be treated as a rewrite of an entire paragraph!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Krzysztof, thanks for reviewing and I'm sorry I didn't get to it until now. If this section should be somewhere else, I'd be happy to move it -- Do you know the best way to ask where to add a guide like this to another package (or if the folks at gha_build would be open to content of this sort in their readme for example)? I'm thinking of copying the (edited based on your suggestions) instructions into a Confluence page in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confluence isn't recommended for documentation (though I don't remember ATM why). I think a readme in gha_build
is a natural place, given that it doesn't have LSST-style docs, but you could ask on #square-docs-support
or #dm-science-pipelines
?
doc/playbook.rst
Outdated
|
||
#. Check the `Science Pipelines changelog <https://lsst-dm.github.io/lsst_git_changelog/weekly/>`_ to make sure only the intended changes are on ``main``. | ||
|
||
If ``main`` contains only changes that are intended for the Science Pipelines build, proceed and build the main branch! Otherwise, a cherry-pick or backport branch should be built. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given my next comment, I assume this is actually backport branches in each Science Pipelines package with changes of interest. If this is correct, please say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression from talking through this with @isullivan was that, if you ended up backporting changes, you'd need a branch containing changes from each of them as you said. I'd love confirmation that that is the case, since I've not gone through this part of the process myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @kfindeisen is correct that you need a backport branch for each package with changes of interest. You do not need to make a branch when main
is already correct (i.e. includes all of the changes you want, and does not have unrelated changes you do not want).
doc/playbook.rst
Outdated
In the future, there may be "patched weekly" builds, which would justify a patch version of Prompt Processing. | ||
If you are planning to update the Science Pipelines tag, you should also check the `Science Pipelines changelog <https://lsst-dm.github.io/lsst_git_changelog/weekly/>`_. | ||
In practice, almost any Science Pipelines update is at least a minor version, because new features are added constantly. | ||
In the future, there may be "patched weekly" builds, which would justify a patch version of Prompt Processing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could swear we removed this line, but now it should probably refer to quick-stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! It is done. Do you know how to link sub-headers in .rst?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default link (i.e., the verbatim title) should just work. An explicit label definitely will.
doc/playbook.rst
Outdated
3. Tag the release | ||
#. Tag the release | ||
|
||
At the HEAD of the ``main`` branch, create and push a tag with the semantic version: | ||
At the HEAD of the ``main`` branch, create and push a tag with the semantic version: | ||
|
||
.. code-block:: sh | ||
.. code-block:: sh | ||
|
||
git tag -s X.Y.Z -m "X.Y.Z" | ||
git push --tags | ||
git tag -s X.Y.Z -m "X.Y.Z" | ||
git push --tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend removing the step 3 block entirely, as it's confused every newcomer to the build process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I think "how to tag something on GitHub" is google-able enough. Maybe we find a nice how-to guide from GitHub and link it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this one? I'll link a doc page when we can find an appropriate how-to. https://git-scm.com/book/en/v2/Git-Basics-Tagging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no: my point is that the GitHub release process (step 2) creates a tag once you publish the release. Manually making the tag is unnecessary at best (that's the confusion I mentioned in my original comment).
* If there are no changes (typically because you want to use an updated Science Pipelines container), go to the repository's `Actions tab <https://github.com/lsst-dm/prompt_processing/actions/workflows/build-base.yml>`_ and select "**Run workflow**". From the dropdown menu, select: | ||
|
||
#. The branch whose container definition will be used | ||
#. The label of the Science Pipelines container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to include an entry for the science pipelines container (possibly adding it on commit "Add directions to playbook..."?) AFAICT that's the step you and Erin missed over the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. [Sadly I was mostly out that weekend so I'll check in with @erinleighh about the specifics]. Do you mean that this set of directions needs the step where the container label is taken from the appropriate container at this link: https://github.com/orgs/lsst-dm/packages?repo_name=prompt_processing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the action is hardcoded to always build prompt-base
, and the container label goes under "Science Pipelines tag" as usual.
But the "Science Pipelines container" needs to have the name of the input container being used to make the base -- lsstsqre/centos
, the default, is the official weekly/daily Science Pipelines build, while Quick-Stack is ghcr.io/lsst/quick-stack
(you mentioned it below, as the output of the build process).
62e1136
to
ca47f95
Compare
Resolved some of the comments above and rebased. Still thinking on some others; see comment replies. Thanks for the review again! |
40480b2
to
1b1ab32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, but please make sure that "Fix numbering for Release Management playbook section" is only changing the formatting and not the content -- the text has changed on main
since December!
Building the Science Pipelines Manually (aka "Quick-Stack Build") | ||
----------------------------------------------------------------- | ||
It will sometimes be necessary to compile a container with the LSST Science Pipelines manually. | ||
It will sometimes be necessary to compile a container with the LSST Science Pipelines manually (called a Quick-Stack build). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit largely reverses "Add directions to playbook for Science Pipelines quick-builds ". Would it be possible to merge the two so that the commit history looks more coherent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant merge only those two commits; now it's impossible to tell the formatting changes from the content ones.
doc/playbook.rst
Outdated
@@ -34,6 +34,7 @@ To build the base container: | |||
|
|||
#. The branch whose container definition will be used | |||
#. The label of the Science Pipelines container. | |||
#. If using a quick-stack build, the Science Pipelines Container shuold be set to `ghcr.io/lsst/quick-stack <ghcr.io/lsst/quick-stack>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#. If using a quick-stack build, the Science Pipelines Container shuold be set to `ghcr.io/lsst/quick-stack <ghcr.io/lsst/quick-stack>`_. | |
#. If using a quick-stack build, the Science Pipelines Container should be set to `ghcr.io/lsst/quick-stack <https://ghcr.io/lsst/quick-stack>`_. |
Do proofread (try following) your links!
Select a tag for the container that reflects its contents (e.g. ``d_2024_06_21_DM-44996`` or ``or4_pp_20240625``). The rubin-env version will be appended. | ||
|
||
Click "**Run Workflow**". The resulting container, if things succeed, will be available at ``ghcr.io/lsst/quick-stack:{your tag here}-{rubin-env version}``. | ||
Instructions for building the Science Pipelines are `here <https://github.com/lsst/gha_build/blob/main/README.md>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention that, for PP, we only need to build lsst_distrib
?
doc/playbook.rst
Outdated
|
||
For the ``prompt_processing`` service, a new major version is triggered by any of the following: | ||
**The following changes will always trigger a major release:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove the "always"? The list is supposed to be exhaustive, and the current wording makes it sound like there are other cases that sometimes justify a major version increment.
doc/playbook.rst
Outdated
.. _DMTN-093: https://dmtn-093.lsst.io/#alertmanagement | ||
* Any specific motivation for the release (for example, including a specific feature, preparing for a specific observing run) | ||
* Science Pipelines version and rubin-env version | ||
* Supported `APDB schema`_ and `ApdbSql`_/`ApdbCassandra`_ versions (see `DMTN-269`_ for rationale). | ||
You do *not* need to consider the `ApdbCassandraReplica` version. | ||
A stack quoting a given minor version is compatible with *older* APDBs of that major version but not necessarily newer ones; for example, a release whose baseline is APDB schema 1.4.0 can access a schema 1.0.0 or 1.4.1 database, but not schema 1.5. | ||
* Supported `Butler dimensions-config`_ versions | ||
* The `Alerts schema`_ version used for output (see `DMTN-093`_ for details) | ||
|
||
.. _DMTN-269: https://dmtn-269.lsst.io/ | ||
.. _DMTN-093: https://dmtn-093.lsst.io/#alertmanagement | ||
|
||
.. _Butler dimensions-config: https://pipelines.lsst.io/v/daily/modules/lsst.daf.butler/dimensions.html#dimension-universe-change-history | ||
.. _DMTN-269: https://dmtn-269.lsst.io/ | ||
|
||
.. _APDB schema: https://github.com/lsst/sdm_schemas/blob/main/python/lsst/sdm/schemas/apdb.yaml#L4 | ||
.. _Butler dimensions-config: https://pipelines.lsst.io/v/daily/modules/lsst.daf.butler/dimensions.html#dimension-universe-change-history | ||
|
||
.. _ApdbSql: https://github.com/lsst/dax_apdb/blob/main/python/lsst/dax/apdb/sql/apdbSql.py#L71-L75 | ||
.. _APDB schema: https://github.com/lsst/sdm_schemas/blob/main/python/lsst/sdm_schemas/schemas/apdb.yaml#L4 | ||
|
||
.. _ApdbCassandra: https://github.com/lsst/dax_apdb/blob/main/python/lsst/dax/apdb/cassandra/apdbCassandra.py#L84-L88 | ||
.. _ApdbSql: https://github.com/lsst/dax_apdb/blob/main/python/lsst/dax/apdb/sql/apdbSql.py#L72-L76 | ||
|
||
.. _ApdbCassandraReplica: https://github.com/lsst/dax_apdb/blob/main/python/lsst/dax/apdb/cassandra/apdbCassandraReplica.py#L52-L56 | ||
.. _ApdbCassandra: https://github.com/lsst/dax_apdb/blob/main/python/lsst/dax/apdb/cassandra/apdbCassandra.py#L85-L89 | ||
|
||
.. _Alerts schema: https://github.com/lsst/alert_packet/blob/main/python/lsst/alert/packet/schema/latest.txt | ||
.. _Alerts schema: https://github.com/lsst/alert_packet/blob/main/python/lsst/alert/packet/schema/latest.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double-check these links, I think you overwrote the fixes from cbb1233 when rebasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this -- I should have been more thorough in my rebase. I'm gonna try some GitHub wizardry to set this back to how it was, hopefully it's minimally messy.
doc/playbook.rst
Outdated
* Any specific motivation for the release (for example, including a specific feature, preparing for a specific observing run) | ||
* Science Pipelines version and rubin-env version | ||
* Supported `APDB schema`_ and `ApdbSql`_/`ApdbCassandra`_ versions (see `DMTN-269`_ for rationale). | ||
You do *not* need to consider the `ApdbCassandraReplica` version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same story -- ApdbCassandraReplica
now matters, please use the wording on main
.
To the right of the list of files, click **Releases**. | ||
At the top of the page, click **Draft a new release**. | ||
Type a tag using semantic versioning described in the previous section. | ||
The Target should be the main branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally figured out what sometimes causes "Previous tag: auto" to pick the wrong version -- when there aren't any (eligible) PRs since the last tag, it goes back to the version before the most recent PR.
Add a note warning that auto
is unreliable if there are no changes in PP itself?
aa722a2
to
80481c3
Compare
Ok, I think I addressed everything here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments.
doc/playbook.rst
Outdated
|
||
#. The branch whose container definition will be used | ||
#. The label of the Science Pipelines container. | ||
#. If using a quick-stack build, the Science Pipelines Container should be set to `ghcr.io/lsst/quick-stack <https://ghcr.io/lsst/quick-stack>`_.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the *
for? It shows up literally in the output.
doc/playbook.rst
Outdated
This will generate a list of commit summaries and of submitters. | ||
Add text as follows. | ||
The `auto` option is unreliable if there are no changes in Prompt Processing itself. | ||
In these cases, if there weren't any eligible Pull Requests since the last tag, it will default to choosing the version before the most recent Pull Request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "In these cases, it may choose an older version than the latest" is enough?
@@ -138,11 +164,6 @@ Add text as follows. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ApdbCassandra
link has bitrotted again. 😂 Should now be #L87-L91
.
cdc98cb
to
0eb1842
Compare
No description provided.